Skip to content

test(linalg): cover the hamming kernel remainder loops per width - #8870

Open
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:test/hamming-kernel-tails
Open

test(linalg): cover the hamming kernel remainder loops per width#8870
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:test/hamming-kernel-tails

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What this changes

Replaces test_hamming_batch_u64 with an rstest over 13 widths that asserts every output slot, and adds a direct test for hamming_batch_avx2's remainder loop. Net +82/-10, tests only.

Why, stated narrowly

I started this believing no test ever executed the kernels' remainder loops. That was wrong, and I checked it rather than shipping the claim: the pairwise tests reach them, because a row sweep passes every width from n - 1 down to 1. Against upstream/main, replacing the scalar remainder loop with while false fails 3 existing tests, and chunks = n / 8 to n.div_ceil(8) fails 5.

So what this actually buys is two narrower things.

Per-width assertions. The old test used n = 128 and checked 4 of 128 slots against hardcoded values. A tail bug today surfaces as a mismatch somewhere among 499,500 pairs; this names the width and the slot.

One real gap. hamming_batch_simd returns early on the AVX-512 branch when avx512vpopcntdq and avx512f are both present, so on a VPOPCNTDQ host nothing reaches hamming_batch_avx2 through hamming_batch_u64. Its only other callers are test_avx2_popcount at n = 8 and test_avx2_max_distance at n = 4, both chunk-exact, so on such a host the AVX2 remainder loop is executed by no test at all. test_avx2_covers_tail closes that.

What the helper does

check_kernel_tail allocates n + 8 output slots, hands the kernel only &mut results[..n], checks every value against (query ^ target).count_ones(), and checks the 8 guard slots. 8 is the widest chunk store in the file, so one extra chunk iteration lands inside the allocation; two would still escape it, and the remainder loops cannot trip the guards at all since all three index through bounds-checked results[..] and would panic first. The guard is best-effort rather than guaranteed, because a store past the reborrowed slice is undefined behaviour the compiler may assume away.

Query and target values are 0xDEAD_BEEF_CAFE_F00D against golden-ratio multiples, rather than the old query = 0 with targets = 0..128. The old data set only the low 7 bits, so _mm256_shuffle_epi8 saw two distinct nibble values and _mm256_sad_epu8 summed a single nonzero byte lane.

What I removed after review

An test_avx512_covers_tail that added nothing. Its feature gate is exactly the dispatch condition, so it only ran where hamming_batch_u64 already drives that kernel, over a wider set of widths through the same helper and oracle. Its one unique case, n = 6, moved into the dispatched rstest, which also closes a gap I had left: no case satisfied n % 8 == 6, so the unrolled scalar fallback that runs on the ARM legs never finished with six leftover slots.

Test plan

  • cargo test --profile ci -p lance-linalg --lib distance::hamming: 56 passed on aarch64, 65 on x86_64-apple-darwin
  • cargo clippy -p lance-linalg --all-targets -- -D warnings and the same for --target x86_64-apple-darwin: clean
  • cargo fmt --all -- --check: clean
  • Mutation, on the scalar kernel because it is the one this host runs: killing the remainder loop fails cases 1, 3, 4, 5, 7, 9, 15, 17 and leaves 8, 16 and 128 green, which are the exact multiples; n / 8 to n.div_ceil(8) fails 1, 4, 9, 15

Not covered: this machine is aarch64 and the x86_64 target runs under Rosetta, which reports no AVX2, so test_avx2_covers_tail is compile-checked here and first executes on an x86 runner. Nothing in CI pins avx512vpopcntdq, which is why the AVX-512 kernel gets no direct test rather than an unreliable one.

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer chore labels Aug 29, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: approve.

These focused tests make the existing kernel-tail contract explicit: the dispatched matrix covers every width-8 remainder, and the direct AVX2 matrix covers every width-4 remainder even when AVX-512 wins dispatch. The all-slot scalar oracle is proportionate for this test-only change.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 29, 2026
@LuciferYang

Copy link
Copy Markdown
Contributor Author

The red linux-arm job is index::vector::ivf::v2::tests::test_create_ivf_hnsw_pq_multivec at recall 0.4, the unseeded k-means flake tracked in #8764 (4 failures in 150 runs on unmodified main, fix waiting in #8767). This PR changes only #[cfg(test)] code in lance-linalg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer chore K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant